Match date/time formatter output to ICU formatting #759
Conversation
ICU uses a narrow non-breaking space (0x202F) in formatted date/time strings, which is based on CLDR. This change makes the expected output for time and datetime match what ICU outputs in these cases.
|
A discussion could be had about what these tests should test for, since the spec doesn't specify exactly how dates/times are formatted. But if the tests are going to be used for checking implementations' conformance, and the expected output of test cases is a single hard-coded string, I think this is the right change. Maybe in the longer term the expected output should be specified differently. The What I want to use the tests for is to make sure my implementation is handling all the options to |
|
I think all of these tests need to be refactored--and not to contain any date/time formats at all. The tests for built-in functions should test that there is a required function, that it accepts the required literals (implementation defined types are up to the implementation), that it accepts the required options with the values specified by the spec, and that it emits the errors required. The exception to this might be the string formatter. If we want to test actual formatting, that belongs in ICU or maybe CLDR. |
There was a problem hiding this comment.
This specific change is in fact a rather good example of why we should not try to match the specific output of ICU. The change in the space character separating the AM/PM marker was made in CLDR 42, and prompted the following browser patches:
- https://phabricator.services.mozilla.com/D160188
- https://bugs.chromium.org/p/v8/issues/detail?id=13494
- WebKit/WebKit#6556
If you test your code with Node.js 20 or later, you should find that it'll use a plain space rather than \u202f.
|
So should we just remove the expected output for the tests in |
|
(as contributor) Yes, that's my opinion. Eventually a bit more than that. But not create tests that depend on the implementation or on specific CLDR data in this particular test suite. |
|
Closing in favor of #760 |
4 participants
In trying to integrate the spec tests directly into the ICU4C tests (without modifying the JSON files), I noticed an inconsistency between the
timeanddatetimefunctions' output as specified in the test, and what ICU outputs. (I'd noticed this before, but as I was importing the tests manually, I changed the expected output manually.)ICU puts a narrow non-breaking space (0x202F) before the "AM" or "PM" in formatted date/time strings, which is a requirement from CLDR. I did a quick test that JS (at least running in Node) does the same thing, and it does:
This change makes the expected output for time and datetime match what ICU outputs in these cases.